Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(engine): added resource to easily configure magic numbers in solver #1379

Merged
merged 1 commit into from
Nov 29, 2024
Merged

feat(engine): added resource to easily configure magic numbers in solver #1379

merged 1 commit into from
Nov 29, 2024

Conversation

GCeSilva
Copy link
Contributor

Description

Added a resource to easily configure magic numbers in solver.

Checklist

  • Self-review changes.
  • Evaluate impact on the documentation.
  • Ensure test coverage.
  • Write new samples.
  • Add entry to the changelog's unreleased section.

@GCeSilva GCeSilva linked an issue Nov 21, 2024 that may be closed by this pull request
Make all magic numbers in the solver easily configurable #1281
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

PR Preview Action v1.4.8
🚀 Deployed preview to https://GameDevTecnico.github.io/cubos/preview/pr-1379/
on branch gh-pages at 2024-11-29 19:15 UTC

@codecov Codecov
Copy link

codecov bot commented Nov 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 37 lines in your changes missing coverage. Please review.

Project coverage is 53.40%. Comparing base (515b9fd) to head (91355be).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...c/physics/solver/penetration_constraint/plugin.cpp 0.00% 17 Missing ⚠️
engine/src/physics/plugin.cpp 0.00% 14 Missing ⚠️
engine/src/physics/solver/integration/plugin.cpp 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1379      +/-   ##
==========================================
- Coverage   53.43%   53.40%   -0.04%     
==========================================
  Files         451      451              
  Lines       25827    25844      +17     
  Branches     2390     2390              
==========================================
+ Hits        13800    13801       +1     
- Misses      12027    12043      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GCeSilva GCeSilva force-pushed the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch from 31567f9 to 76f250b Compare November 21, 2024 18:50
@GCeSilva GCeSilva marked this pull request as ready for review November 21, 2024 19:15
@GCeSilva GCeSilva requested review from RiscadoA and a team as code owners November 21, 2024 19:15
@GCeSilva GCeSilva requested review from SrGesus, fallenatlas and joaomanita and removed request for a team November 21, 2024 19:15
@cubos-bot Cubos Bot
Copy link

cubos-bot bot commented Nov 21, 2024

@SrGesus will only be able to review this PR in 5 days.

@RiscadoA RiscadoA added this to the 0.5 milestone Nov 23, 2024
fallenatlas
fallenatlas requested changes Nov 23, 2024
View reviewed changes
Copy link
Contributor

@fallenatlas fallenatlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! 👍 Overall looks good to me, just have a couple of details that need sorting out.

engine/src/physics/solver/integration/physics_constants_integration.hpp Outdated Show resolved Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp Outdated Show resolved Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp Outdated Show resolved Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp Outdated Show resolved Hide resolved
@GCeSilva GCeSilva force-pushed the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch 2 times, most recently from 14d27c5 to 9dab08d Compare November 24, 2024 15:13
@RiscadoA RiscadoA removed the request for review from SrGesus November 25, 2024 08:52
RiscadoA
RiscadoA approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@RiscadoA RiscadoA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than what I pointed out LGTM! Ty for working on this!
If you could open a follow up issue to add a tool for configuring these values at runtime it would be cool 👀 might be a good first issue for someone interesting in playing with tools, or for one of you in the physics team if you want to be able to properly debug stuff

engine/src/physics/solver/integration/plugin.cpp Outdated Show resolved Hide resolved
engine/src/physics/solver/integration/physics_constants_integration.hpp Outdated Show resolved Hide resolved
engine/src/physics/solver/penetration_constraint/physics_constants_pc.hpp Outdated Show resolved Hide resolved
engine/src/physics/solver/integration/physics_constants_integration.hpp Outdated Show resolved Hide resolved
@RiscadoA
Copy link
Member

Also, don't forget to update the CHANGELOG.md!

@GCeSilva GCeSilva force-pushed the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch 2 times, most recently from 7b33cd3 to 96f8578 Compare November 29, 2024 16:18
@GCeSilva GCeSilva requested a review from a team as a code owner November 29, 2024 16:18
@GCeSilva GCeSilva requested review from fallenatlas and removed request for a team November 29, 2024 16:18
fallenatlas
fallenatlas approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@fallenatlas fallenatlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍 Don't forget to update the CHANGELOG

@GCeSilva GCeSilva force-pushed the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch from 96f8578 to 7227aef Compare November 29, 2024 19:14
@GCeSilva GCeSilva force-pushed the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch from 7227aef to 91355be Compare November 29, 2024 19:21
@GCeSilva GCeSilva enabled auto-merge (rebase) November 29, 2024 19:24
@GCeSilva GCeSilva merged commit b7c2823 into main Nov 29, 2024
9 checks passed
@GCeSilva GCeSilva deleted the 1281-make-all-magic-numbers-in-the-solver-easily-configurable branch November 29, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaomanita joaomanita joaomanita approved these changes

@fallenatlas fallenatlas fallenatlas approved these changes

@RiscadoA RiscadoA RiscadoA approved these changes

Assignees
No one assigned
Labels
A-Engine B-Physics
Projects
None yet
Milestone
0.5
Development

Successfully merging this pull request may close these issues.

Make all magic numbers in the solver easily configurable
4 participants
@GCeSilva @RiscadoA @fallenatlas @joaomanita